etcdctl: add etcdctl snapshot pipe command #16243
Conversation
Ais8Ooz8
force-pushed
the
main
branch
2 times, most recently
from
ab01e0c to
11fb917
Compare
There was a problem hiding this comment.
Thanks for putting this proposal forward @Ais8Ooz8.
My initial feeling is that there is now unnecessary duplication here. Just as an idea we could maybe mostly refactor the SaveWithVersion and PipeWithVersion functions into a single new SaveSnapshotWithVersion function that accepts an additional io writer / file parameter?
This way, we could reuse most of the logic while providing flexibility in saving the snapshot to different destinations, such as an actual file or os.Stdout.
The implementations for Save vs Pipe would then become something roughly like this perhaps:
func SaveWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, dbPath string) (string, error) {
file, err := os.OpenFile(dbPath+".part", os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileutil.PrivateFileMode)
if err != nil {
return "", fmt.Errorf("could not open %s (%v)", dbPath+".part", err)
}
defer os.RemoveAll(dbPath + ".part")
defer file.Close()
return SaveSnapshotWithVersion(ctx, lg, cfg, file)
}
func PipeWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config) (string, error) {
return SaveSnapshotWithVersion(ctx, lg, cfg, os.Stdout)
}Just an idea and not tested at all so feel free to hold off until more people review.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions. |
I agree with James. Much of the logic can be refactored by passing a file writer into @Ais8Ooz8 can you followup on this? Thanks! |
etcdctl: add etcdctl snapshot pipe command To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Signed-off-by: Ais8Ooz8 <[email protected]>
|
/ok-to-test |
To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Signed-off-by: Ais8Ooz8 <[email protected]>
|
/ok-to-test |
|
@Ais8Ooz8 can you fix the fmt error? It is a nitty one about an empty line. Thanks! |
To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Signed-off-by: Ais8Ooz8 <[email protected]>
|
/retest |
|
/lgtm |
There was a problem hiding this comment.
Overall looks good to me - Thanks @Ais8Ooz8. A couple small suggestions below if you have time.
Additionally - Given this is user facing new feature I would suggest we add an entry to https://github.com/etcd-io/etcd/blob/main/CHANGELOG/CHANGELOG-3.6.md#etcdctl-v3.
To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Signed-off-by: Ais8Ooz8 <[email protected]>
To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Signed-off-by: Ais8Ooz8 <[email protected]>
|
Discussed during sig-etcd triage - this is looking good, @ahrtr could you please take a look for final review? |
|
Is there a real use case for the |
|
QQ, isn't the UNIX convention to use High level, I don't think we need a separate command, it can be just a flag. |
To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Signed-off-by: Ais8Ooz8 <[email protected]>
To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Signed-off-by: Ais8Ooz8 <[email protected]>
To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Signed-off-by: Ais8Ooz8 <[email protected]>
To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Signed-off-by: Ais8Ooz8 <[email protected]>
|
/retest |
|
@ahrtr Since most encryption and compression utilities work with standard streams, and s3 utilities have subcommands such as @serathius I think tar uses |
|
/retest |
To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Signed-off-by: Ais8Ooz8 <[email protected]>
|
Since @serathius has a comment on adding a flag instead of a new command, so leave to @serathius to take a second look. Either a flag or a new command works for me. A new command is slight clearer, but not a big deal in this case. |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Useful for read-only file systems.
Solves #16242